fix(sqlserver): make EnsureDatabaseExistsAsync safe under concurrent callers (weasel#415) - #418
Merged
Merged
Conversation
…callers (weasel#415) EnsureDatabaseExistsAsync did a check-then-create against master. Two callers both see DB_ID return null, both issue CREATE DATABASE, and the loser gets SqlException 1801, "Database 'x' already exists". The method also returned as soon as CREATE DATABASE completed, so the caller's next OpenAsync against the new catalog could still fail -- a freshly created SQL Server database briefly refuses logins. Neither mattered while a single process bootstrapped a single database at startup. Both matter as soon as parallel test workers each provision their own database against a cold container. The logic is not new. SqlServerDatabaseBootstrap in the EF Core test project already solved exactly this and has been stranded there where nothing outside those tests could reach it. This promotes it into the shipped assembly, and that helper is now a thin shim that delegates. * SqlException 1801 is caught and treated as success. The IF DB_ID(...) IS NULL CREATE DATABASE form was considered and rejected as the whole answer -- SQL Server does not make that pair atomic against a concurrent CREATE, so it only narrows the window. The 1801 catch is what actually closes it, so the cheap parameterized DB_ID check is kept as the fast path and the catch does the real work. * After the create, the method polls until the database accepts a connection. The wait is unconditional rather than only-when-we-created-it, because a concurrent creator leaves us the same window. When it returns, callers can take the postcondition at face value: the database exists and is reachable. * The wait is bounded and expires loudly. TimeoutException names the database and points at the knob, with the last SqlException as InnerException. Silence after a timeout would be worse than the old behaviour. Answering the issue's open questions: * The ceiling is configurable. DatabaseAvailabilityTimeout defaults to 30s (matching the test helper's 30 x 1s) and DatabaseAvailabilityPollingInterval to 1s. TimeSpan.Zero makes a single attempt and fails fast, which is what a developer with a warm container wants. * Other providers: PostgreSQL has the same check-then-create shape and the same race, so PostgresqlMigrator.EnsureDatabaseExistsAsync now catches 42P04 duplicate_database. It needs no availability wait -- Postgres accepts connections to a new database as soon as CREATE DATABASE returns, so that half is genuinely SQL-Server-specific. Oracle/MySql/Sqlite not audited. * EnsureDatabaseExistsAsync stays the home for this. Provisioning N databases is N calls, and the awkwardness of the DbConnection-shaped signature is a separate concern from the race. Also escapes ']' in the database name, since CREATE DATABASE takes no parameters and the name has to be interpolated into a delimited identifier. Regression tests: ensure_database_is_safe_under_concurrent_callers races 8 concurrent callers at one new database and fails against 9.22.0 (verified by disabling only the 1801 catch); the offline-database test pins the timeout message and its inner exception. Full Weasel.SqlServer suite green (343 passed, 8 pre-existing skips), Weasel.EntityFrameworkCore SqlServer tests green (17) on the delegating bootstrap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 31, 2026
erdtsieck
pushed a commit
to erdtsieck/weasel
that referenced
this pull request
Aug 3, 2026
…urrent window enumerating_while_registrations_land_neither_throws_nor_tears raced the writer to the finish: it read in a `while (!writer.IsCompleted)` loop and then asserted `passes > 0`. When the pool scheduled the writer promptly it landed all 5,000 registrations before the first IsCompleted check, so the loop body never ran and the test asserted nothing about the registry -- green locally, and red in CI on the trailing `passes > 0` with `Shouldly.ShouldAssertException : passes` (JasperFx#418, Postgres 15.3-alpine net9.0 job, passed on rerun with no code change). The concurrent window is now established by handshake rather than by scheduling luck. The writer signals after its first registration and the reader blocks on that before taking any pass; the reader then takes a fixed number of passes and signals when done; the writer, after registering its new keys, keeps re-registering them until that signal arrives. It never blocks, so the map is being actively written to for the whole of the reader's passes, and the vacuous `passes > 0` assertion is gone. The invariants the test exists to protect are unchanged: a read never sees fewer entries than were seeded and never sees a null. Re-registering existing keys does not move the final count, so `Count == seeded + added` still holds -- and still fails under the non-atomic read-modify-write setter this test guards against, where a stale-snapshot write during the churn can drop keys outright. Verified with 200 consecutive local runs, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #415.
SqlServerMigrator.EnsureDatabaseExistsAsyncdid a check-then-create againstmaster. Two gaps show up as soon as more than one process calls it at once:DB_IDreturn null, both issueCREATE DATABASE, and the loser getsSqlException1801 — "Database 'x' already exists".CREATE DATABASEcompleted, so the caller's nextOpenAsyncagainst the new catalog could still fail — a freshly created SQL Server database briefly refuses logins.Neither mattered while a single process bootstrapped a single database at startup. Both matter as soon as parallel test workers each provision their own database against a cold container.
Promoting proven code, not writing new logic
SqlServerDatabaseBootstrapinWeasel.EntityFrameworkCore.Testsalready solved exactly this and has been stranded where nothing outside those tests could reach it. This moves it into the shipped assembly; that helper is now a thin shim that delegates.What changed
IF DB_ID(...) IS NULL CREATE DATABASEform was considered and rejected as the whole answer — SQL Server does not make that pair atomic against a concurrentCREATE, so it only narrows the window. The 1801 catch is what closes it, so the cheap parameterizedDB_IDcheck stays as the fast path and the catch does the real work.TimeoutExceptionnames the database, points at the knob, and carries the lastSqlExceptionasInnerException. Silence after a timeout would be worse than the old behaviour.]in the database name is escaped.CREATE DATABASEtakes no parameters, so the name has to be interpolated into a delimited identifier.Answering the issue's open questions
Should the retry ceiling be configurable? Yes.
DatabaseAvailabilityTimeoutdefaults to 30s (matching the test helper's 30 × 1s) andDatabaseAvailabilityPollingIntervalto 1s.TimeSpan.Zeromakes a single attempt and fails fast, which is what a developer with a warm container wants.Do the other providers need the same treatment? PostgreSQL has the same check-then-create shape and the same race, so
PostgresqlMigrator.EnsureDatabaseExistsAsyncnow catches42P04duplicate_database. It needs no availability wait — Postgres accepts connections to a new database as soon asCREATE DATABASEreturns, so that half is genuinely SQL-Server-specific. Oracle/MySql/Sqlite are not audited here.Is
EnsureDatabaseExistsAsyncthe right home? Yes. Provisioning N databases is N calls, and the awkwardness of theDbConnection-shaped signature is a separate concern from the race.Verification
ensure_database_is_safe_under_concurrent_callersraces 8 concurrent callers at one new database. Confirmed failing againstmaster(verified by disabling only the 1801 catch) and passing here.times_out_with_a_clear_message_when_the_database_never_accepts_connectionsuses an offline database as the reproducible stand-in for "created but refusing logins", and pins the message and inner exception.ensure_database_escapes_a_bracket_in_the_database_namecovers the identifier escaping.Weasel.SqlServersuite green: 343 passed, 8 pre-existing skips.Weasel.EntityFrameworkCoreSQL Server tests green (17) running on the delegating bootstrap.🤖 Generated with Claude Code